Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add new dark theme: Dark berry blue #2065

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

ppenguin
Copy link

The number of built-in dark themes is lacking compared to the light themes.
This PR aims to be a first step to alleviate this, in submitting a dark variant of "Mixed Berry Blue".

As a side note: it would be logical to make the IDs of themes a bit more symmetrical: since the ID category seems to be 200 <= DarkThemeID < 300, it is a logical expectation that a dark variant of an existing light theme is LightID + 200?

It would be a reasonable goal to work toward "dark theme coverage" in this sense too?

@alixander
Copy link
Collaborator

alixander commented Aug 24, 2024

@ppenguin Thanks for this PR! Can you screenshot what it looks like?

And no the goal isn't to have a dark theme equivalent to every light theme. There's very few themes I've seen in the software world that have a light and dark version. Even then, if you saw their light and dark versions among others, it wouldn't be obvious they belong to the same theme, so they're kind of just two different themes. D2 should categorize the two separately -- mirrors of light/dark themes are fine as long as they make sense and look good, but no need to have mirrors for every theme.

@ppenguin ppenguin force-pushed the add-dark-theme-darkberry branch from fdbc460 to 1659ebb Compare August 24, 2024 14:38
@ppenguin
Copy link
Author

ppenguin commented Aug 24, 2024

@ppenguin Thanks for this PR! Can you screenshot what it looks like?

Here's a screenshot:
themex-205

And no the goal isn't to have a dark theme equivalent to every light theme. There's very [few themes]
...
so they're kind of just two different themes. D2 should categorize the two separately -- mirrors of light/dark themes are fine as long as they make sense and look good, but no need to have mirrors for every theme.

Makes sense, no need to go overboard, it's just that the choice of "tones" felt kind of incomplete, from gut feeling I think a few more would be nice, this one would go into the category "dark pastel blue/purple".

That would make the status like this:

  • Dark Mauve: Brownish/old-purplish
  • Dark Flagship Terrastruct: Dark sharp blue
  • Dark Berry Blue: Dark pastel blue/purple

and possibly TODO:

  • something dark green-tint
  • something dark "blood"/"vampire"?
  • dark pinkish
  • dark ocre/yellow

This branch has a handy build script to (re)generate an example for all available themes (you can include it in the codebase if you want to).

tweak darkberry theme

t
@ppenguin ppenguin force-pushed the add-dark-theme-darkberry branch from 1659ebb to f4249ce Compare August 27, 2024 14:08
Copy link
Collaborator

@alixander alixander left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good! Can you please add a line to changelog (next.md) of the new theme too (under improvements)?

import "oss.terrastruct.com/d2/d2themes"

var DarkBerryBlue = d2themes.Theme{
ID: 205,
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This makes sense for mirroring themes but I think it may be a confusing precedent now that we agree the themes shouldn't be mirrored. Can you change this to just the next sequential dark theme ID? (202)

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IMHO "agreeing not to mirror all themes" is quite different from "mirroring some themes", and that the DX/UX of using this numbering scheme for themes that are mirrored is arguably better (the fact that category number groups are used does already mean that the numbers are not contiguous, so sequential numbering is a moot point). Instead I'd suggest to change the ID of the "flagship" theme to 203 and the world will be in order again 😉

(Note that it might even have advantages down the road for people who might want to programatically set their own "corporate identity" theme automatically to a user's preference).

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually I was wrong in my original comment about why we shouldn't mirror IDs. There shouldn't be any mirroring at all. D2's theme IDs are blocks of continuous ranges, where each range is a category of themes.

Starting from 0 they are cool.
Starting from 100 they are warm.
Starting from 200 they are dark.

Since it's not binary, mirroring doesn't even make sense. It's very possible 300s start up as something else entirely, like "3d", or "minimal".

In other words, it's not broken up like x = light and 20x = dark where you can make a case for mirroring.


Sequential numbering is not moot just because it's discontinuous between groups.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't see why we couldn't have both, it's a perceived UX bonus at no extra cost (since "faux contiguous range" has no added value at all). But let's agree to disagree, not really worth haggeling over...

@alixander
Copy link
Collaborator

@ppenguin Nice build script! I think that'd be useful as a dev tool in the d2themes package if you want to make a separate PR to help include that. The hardcoded theme value in the d2 file used should be removed though (https://github.com/ppenguin/d2/blob/ppdev/testdata/examples/themex.d2#L6).

No obligation to of course, will keep this in mind and get to it myself down the road if not.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants